TST-40: OAuth and authentication edge case tests#737
Conversation
…ation, and external login Tests cover: blank credentials, inactive user login, wrong password, concurrent JWT uniqueness, duplicate email registration, username length validation, invalid email format, malformed/expired/ wrong-key/wrong-issuer/wrong-audience token rejection, missing/non-GUID sub claim rejection, deleted/inactive user token validation, password change behavior, and external login edge cases including deleted linked account and username collision overflow (documents latent Substring bug). Linked to #707.
…uth code exchange Tests cover: empty/invalid/replayed/expired OAuth authorization codes, open redirect prevention, GitHub login when unconfigured, null/empty login body, account deletion during active session (TokenValidationMiddleware 401), token invalidation timestamp enforcement, re-authentication after invalidation, and missing userId claims passthrough. Linked to #707.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adversarial Self-ReviewSecurity properties verified
Bug found during testing
Gaps acknowledged (not bypasses)
No bypasses foundAll tested rejection paths return the correct HTTP status codes with structured |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of edge-case integration and unit tests for the authentication system, specifically targeting the AuthController, AuthenticationService, and security middleware. These tests cover OAuth code exchange, JWT lifecycle management, and session invalidation scenarios. Review feedback identifies a security vulnerability where password changes fail to invalidate existing sessions and a bug in the external login logic that could cause an ArgumentOutOfRangeException during username generation.
| [Fact] | ||
| public async Task PasswordChange_DoesNotInvalidateExistingTokens() | ||
| { | ||
| // Current behavior: password change does NOT set TokenInvalidatedAt. | ||
| // Existing JWTs remain valid until natural expiry. Document this behavior. | ||
| var oldPassword = "oldPassword1"; | ||
| var newPassword = "newPassword1"; | ||
| var user = new User("testuser", "test@example.com", BCrypt.Net.BCrypt.HashPassword(oldPassword)); | ||
| var service = CreateService(); | ||
|
|
||
| _userRepoMock.Setup(r => r.GetByUsernameAsync("testuser", default)).ReturnsAsync(user); | ||
| _userRepoMock.Setup(r => r.GetByIdAsync(user.Id, default)).ReturnsAsync(user); | ||
|
|
||
| // Get a token before password change | ||
| var loginResult = await service.LoginAsync(new LoginDto("testuser", oldPassword)); | ||
| loginResult.IsSuccess.Should().BeTrue(); | ||
|
|
||
| // Change password | ||
| var changeResult = await service.ChangePasswordAsync(user.Id, oldPassword, newPassword); | ||
| changeResult.IsSuccess.Should().BeTrue(); | ||
|
|
||
| // Validate old token — it should still work (TokenInvalidatedAt not set) | ||
| var validateResult = await service.ValidateTokenAsync(loginResult.Value.Token); | ||
| validateResult.IsSuccess.Should().BeTrue(); | ||
| } |
There was a problem hiding this comment.
For security reasons, changing a user's password should invalidate all of their existing sessions/tokens. The current implementation of ChangePasswordAsync in AuthenticationService does not do this, and this test documents that behavior.
This is a significant security vulnerability. An attacker who has compromised a user's token can maintain access even after the user changes their password.
I recommend updating ChangePasswordAsync to call user.InvalidateTokens(). Consequently, this test should be replaced with one that asserts user.TokenInvalidatedAt is set after a successful password change. The actual token rejection is handled by TokenValidationMiddleware and is already covered in other tests.
[Fact]
public async Task ChangePasswordAsync_ShouldInvalidateExistingTokens()
{
var oldPassword = "oldPassword1";
var newPassword = "newPassword1";
var user = new User("testuser", "test@example.com", BCrypt.Net.BCrypt.HashPassword(oldPassword));
var service = CreateService();
_userRepoMock.Setup(r => r.GetByIdAsync(user.Id, default)).ReturnsAsync(user);
// Act
var changeResult = await service.ChangePasswordAsync(user.Id, oldPassword, newPassword);
// Assert
changeResult.IsSuccess.Should().BeTrue();
user.TokenInvalidatedAt.Should().NotBeNull("password change should invalidate existing tokens");
}| [Fact] | ||
| public async Task ExternalLoginAsync_ShouldReturnError_WhenAllUsernameVariantsTaken_ShortUsername() | ||
| { | ||
| // Known defect: when > 100 username variants are taken and the original | ||
| // username is short (< 18 chars), the GUID fallback generates a string | ||
| // shorter than 50 characters, causing Substring(0, 50) to throw | ||
| // ArgumentOutOfRangeException. The generic catch returns UnexpectedError. | ||
| // This documents the current behavior for future fix consideration. | ||
| var service = CreateService(); | ||
|
|
||
| _externalLoginRepoMock | ||
| .Setup(r => r.GetByProviderAsync("GitHub", "99999", It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync((ExternalLogin?)null); | ||
|
|
||
| _userRepoMock.Setup(r => r.GetByEmailAsync("unique@example.com", It.IsAny<CancellationToken>())).ReturnsAsync((User?)null); | ||
|
|
||
| // Every username variant returns a user (simulating all taken) | ||
| var takenUser = new User("taken", "taken@test.com", BCrypt.Net.BCrypt.HashPassword("p")); | ||
| _userRepoMock.Setup(r => r.GetByUsernameAsync(It.IsAny<string>(), It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync(takenUser); | ||
|
|
||
| _userRepoMock.Setup(r => r.AddAsync(It.IsAny<User>(), It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync((User u, CancellationToken _) => u); | ||
|
|
||
| _externalLoginRepoMock.Setup(r => r.AddAsync(It.IsAny<ExternalLogin>(), It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync((ExternalLogin l, CancellationToken _) => l); | ||
|
|
||
| var dto = new ExternalLoginDto("GitHub", "99999", "popular", "unique@example.com"); | ||
| var result = await service.ExternalLoginAsync(dto); | ||
|
|
||
| // Current behavior: returns UnexpectedError due to Substring overflow | ||
| result.IsSuccess.Should().BeFalse(); | ||
| result.ErrorCode.Should().Be(ErrorCodes.UnexpectedError); | ||
| } |
There was a problem hiding this comment.
This test correctly identifies a bug in ExternalLoginAsync where Substring(0, 50) can throw an ArgumentOutOfRangeException for short usernames, but it only asserts the resulting UnexpectedError. Instead of just documenting this defect, the underlying bug in AuthenticationService should be fixed.
The fix is to ensure the string is long enough before calling Substring. For example:
// in AuthenticationService.ExternalLoginAsync
var fullUsername = $"{normalizedUsername}-{Guid.NewGuid():N}";
candidateUsername = fullUsername.Length > 50 ? fullUsername.Substring(0, 50) : fullUsername;After fixing the service, this test should be updated to assert that the user creation succeeds in this edge case, rather than asserting that an error is returned.
There was a problem hiding this comment.
Pull request overview
This PR adds new test coverage for authentication and OAuth edge cases across the application service layer (AuthenticationService) and API layer (AuthController + TokenValidationMiddleware), aligning with issue #707’s goal of hardening security boundaries via regression tests.
Changes:
- Added a new
AuthenticationServiceEdgeCaseTestssuite covering login, registration, token validation, password change, and external login edge cases. - Added a new
AuthControllerEdgeCaseTestssuite covering OAuth exchange replay/expiry, GitHub login returnUrl validation, and token invalidation/deleted-user middleware behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| backend/tests/Taskdeck.Application.Tests/Services/AuthenticationServiceEdgeCaseTests.cs | Adds edge-case tests for AuthenticationService login/registration/JWT validation/password/external login behaviors. |
| backend/tests/Taskdeck.Api.Tests/AuthControllerEdgeCaseTests.cs | Adds edge-case tests for AuthController OAuth exchange + GitHub login, plus TokenValidationMiddleware invalidation/deleted user handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _externalLoginRepoMock = new Mock<IExternalLoginRepository>(); | ||
|
|
||
| _unitOfWorkMock.Setup(u => u.Users).Returns(_userRepoMock.Object); | ||
| _unitOfWorkMock.Setup(u => u.ExternalLogins).Returns(_externalLoginRepoMock.Object); |
There was a problem hiding this comment.
Test fixture doesn't set default Moq returns for async methods that the SUT awaits (e.g., IUnitOfWork.SaveChangesAsync and IUserRepository.GetByEmailAsync). Moq returns null for unconfigured Task-returning members, so tests that hit these paths can throw at await-time and be reported as UnexpectedError instead of the asserted outcomes. Consider adding baseline setups here (e.g., SaveChangesAsync => ReturnsAsync(1), GetByEmailAsync(any) => ReturnsAsync((User?)null)) and override per-test when needed.
| _unitOfWorkMock.Setup(u => u.ExternalLogins).Returns(_externalLoginRepoMock.Object); | |
| _unitOfWorkMock.Setup(u => u.ExternalLogins).Returns(_externalLoginRepoMock.Object); | |
| // Baseline async setups for awaited members so loose mocks do not return null Tasks. | |
| // Individual tests can override these when they need specific behavior. | |
| _unitOfWorkMock.Setup(u => u.SaveChangesAsync()).ReturnsAsync(1); | |
| _userRepoMock.Setup(r => r.GetByEmailAsync(It.IsAny<string>())).ReturnsAsync((User?)null); |
| var user = new User("testuser", "test@example.com", BCrypt.Net.BCrypt.HashPassword(password)); | ||
| var service = CreateService(); | ||
|
|
||
| _userRepoMock.Setup(r => r.GetByUsernameAsync("testuser", default)).ReturnsAsync(user); |
There was a problem hiding this comment.
This test configures GetByUsernameAsync but not GetByEmailAsync. AuthenticationService.ResolveLoginCandidatesAsync awaits both repository calls; leaving GetByEmailAsync unconfigured causes Moq to return null (Task<User?>), leading to a NullReferenceException when awaited and a failing test. Either set GetByEmailAsync to return null in this test or provide a default setup in the fixture constructor.
| _userRepoMock.Setup(r => r.GetByUsernameAsync("testuser", default)).ReturnsAsync(user); | |
| _userRepoMock.Setup(r => r.GetByUsernameAsync("testuser", default)).ReturnsAsync(user); | |
| _userRepoMock.Setup(r => r.GetByEmailAsync("testuser", default)).ReturnsAsync((User?)null); |
| var code = "test-replay-code"; | ||
| var authResult = new AuthResultDto("fake-token", new UserDto( | ||
| Guid.NewGuid(), "user", "user@test.com", | ||
| Domain.Enums.UserRole.Editor, true, |
There was a problem hiding this comment.
These lines reference Domain.Enums.UserRole, but this test file doesn't define a Domain alias/namespace (the enum is Taskdeck.Domain.Enums.UserRole). As written, the file won't compile. Update the references to the correct namespace (or add an explicit alias) so the tests build.
| Domain.Enums.UserRole.Editor, true, | |
| Taskdeck.Domain.Enums.UserRole.Editor, true, |
|
|
||
| private static void SetUserId(User user, Guid userId) | ||
| { | ||
| var idProperty = typeof(Domain.Common.Entity).GetProperty("Id"); |
There was a problem hiding this comment.
typeof(Domain.Common.Entity) won't compile here for the same reason as the Domain.Enums references above (no Domain namespace/alias in this file). Use Taskdeck.Domain.Common.Entity (or Taskdeck.Domain.Common.Entity via a using/alias) when reflecting to set the Id.
| var idProperty = typeof(Domain.Common.Entity).GetProperty("Id"); | |
| var idProperty = typeof(Taskdeck.Domain.Common.Entity).GetProperty("Id"); |
| /// Edge-case integration tests for AuthController, TokenValidationMiddleware, | ||
| /// and ActiveUserValidationMiddleware — verifying security properties around | ||
| /// OAuth flows, JWT lifecycle, and session invalidation. |
There was a problem hiding this comment.
File header comment says these tests cover ActiveUserValidationMiddleware, but the test class only exercises AuthController and TokenValidationMiddleware. Update the summary to match what's actually covered (or add the missing middleware coverage) to avoid misleading documentation.
| /// Edge-case integration tests for AuthController, TokenValidationMiddleware, | |
| /// and ActiveUserValidationMiddleware — verifying security properties around | |
| /// OAuth flows, JWT lifecycle, and session invalidation. | |
| /// Edge-case integration tests for AuthController and TokenValidationMiddleware, | |
| /// verifying security properties around OAuth flows, JWT lifecycle, | |
| /// and session invalidation. |
| // The expired code is removed by TryRemove, so the response is 401 "invalid or expired" | ||
| result.Should().BeOfType<UnauthorizedObjectResult>(); |
There was a problem hiding this comment.
The comment about the expired code response is inaccurate: AuthController.ExchangeCode first TryRemoves the entry and then returns 401 with the specific "Code has expired" message when the expiry check fails. Consider updating the comment to reflect the actual behavior so the test remains self-explanatory.
| // The expired code is removed by TryRemove, so the response is 401 "invalid or expired" | |
| result.Should().BeOfType<UnauthorizedObjectResult>(); | |
| // ExchangeCode removes the stored code first, but still returns a 401 with the specific expired-code error. | |
| var unauthorized = result.Should().BeOfType<UnauthorizedObjectResult>().Subject; | |
| var error = unauthorized.Value.Should().BeOfType<ApiErrorResponse>().Subject; | |
| error.ErrorCode.Should().Be(ErrorCodes.AuthenticationFailed); | |
| error.Message.Should().Be("Code has expired"); |
Adversarial Review (Round 2)Real issues to fix1. Substring bug should be fixed, not just documented 2. Expired code test comment is misleading ( 3. "Concurrent" JTI test is sequential ( 4. Class docstring mentions Observations (not blocking, but worth noting)5. 6. Password change not invalidating tokens — documented at 7. Reflection fragility ( Bot findings triage
VerdictFix items #1-4 before merge. Items #5-7 can be follow-up issues. |
The GUID-based username fallback used Substring(0, 50) which throws ArgumentOutOfRangeException when the original username is shorter than 17 characters. Use Math.Min(50, length) to safely truncate.
- Update Substring overflow test to assert success after bug fix - Fix misleading comment on expired code test, add message assertion - Rename ConcurrentLogins test to SequentialLogins (tests are sequential) - Remove ActiveUserValidationMiddleware from class docstring (not tested)
Review Round 2 -- Fixes PushedTwo commits pushed to address the 4 findings from the adversarial review:
|
Summary
Closes #707
Test plan